Change `JSClosure.release` to `deinit` #33

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Merged

kateinoigakukun merged 4 commits into master from jsclosure-deinit

Aug 11, 2020

Merged

Change `JSClosure.release` to `deinit` #33

kateinoigakukun merged 4 commits into master from jsclosure-deinit

Aug 11, 2020

Conversation

@MaxDesiatov

Copy link

Member

@MaxDesiatov MaxDesiatov commented Aug 4, 2020 •

edited

Loading

Consider this test case:

func closureScope() -> ObjectIdentifier {
 let closure = JSClosure { _ in .undefined }
 return ObjectIdentifier(closure)
}
Closure_Identifiers: do {
 let oid1 = closureScope()
 let oid2 = closureScope()
 try expectEqual(oid1, oid2)
} catch {
 print(error)
}

It will pass and it is reasonable to expect that: within closureScope the closure reference has its reference count decreased to zero as soon as the function returns. The second time closureScope is called, the new closure instance is allocated at the same memory address as long as you don't allocate anything new on the heap between both closureScope calls. That memory address is used as the result of ObjectIdentifier. The side effect of this behavior manifests itself in this declaration in the body of the JSClosure class:

static var sharedFunctions: [JavaScriptHostFuncRef: ([JSValue]) -> JSValue] = [:]

where in JSClosure.init the keys of this dictionary are taken from ObjectIdentifier values:

let objectId = ObjectIdentifier(self)
let funcRef = JavaScriptHostFuncRef(bitPattern: Int32(objectId.hashValue))
Self.sharedFunctions[funcRef] = body

Currently the users of JavaScriptKit have to manually call JSClosure.release() to clean up this dictionary. This is error-prone and leads to bugs that are hard to diagnose. As far as I understand, the current test suite doesn't have access to a proper DOM environment to reproduce this fully, but consider this more complex case:

func observer(x: Int) {
 let closure = JSClosure { _ in print(x) }
 _ = JSObjectRef.global.document.object!.addEventListener!("visibilitychange", closure)
}
observe(x: 1)
observe(x: 2)

You'd expect this to print 1 and then 2 sequentially when the observer is triggered. That's not the case and you get 2 printed twice since the second closure overwrites the first closure in the sharedFunctions dictionary because both of them have the same ObjectIdentifier value. The author of this code should have retained the closure for the expected lifetime of the observer to get different ObjectIdentifier values, and then they should have called release() on those closures manually after that. They (well, in some cases that was me 😄) failed to do so, and JavaScriptKit didn't warn them about it.

What I propose is changing release() to deinit, while keeping its body. In the observer case both closures would be deallocated instantly, but at least a user would get a crash as soon as the observer triggered. This would notify them of the bug and give them a chance to fix it.

Unfortunately, there's no way for us to be notified on the Swift side when closures are deallocated on the JavaScript side. JavaScript doesn't have finalizers, or destructors, or whatever you'd call it. So there's no way to fully fix this behavior and make it seamless. I'm afraid the crash at run time is the best I can propose so far, but at least it's better than a subtle bug that's hard to notice and hard to diagnose.

MaxDesiatov added 3 commits

August 4, 2020 23:01

@MaxDesiatov


 Change JSClosure.release to deinit

4082c87

@MaxDesiatov


 Rename test case in PrimaryTests

498b6ba

@MaxDesiatov


 Add newline to PrimaryTests/main.swift

b4042fa

@MaxDesiatov MaxDesiatov requested review from carson-katri, j-f1 and kateinoigakukun

August 4, 2020 22:18

@MaxDesiatov

Copy link

Member Author

MaxDesiatov commented Aug 4, 2020 •

edited

Loading

@carson-katri assigning you for review too, as I think if this is merged it will impact a lot of things in Tokamak.

@j-f1

Copy link

Member

j-f1 commented Aug 8, 2020

There’s FinalizationRegistry which is supported everywhere but Safari and allows you to probably (at the engine’s discretion) have a callback called when a given object is garbage collected. If this is available, it might be possible to combine it with WeakRefs to release the closure on the Swift side too.

kateinoigakukun

kateinoigakukun requested changes

Aug 9, 2020

View reviewed changes

Copy link

Member

@kateinoigakukun kateinoigakukun left a comment •

edited

Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JSClosure.release shouldn't be called at deinit because it will break call of Swift function from JavaScript.

func observer(x: Int) {
 let closure = JSClosure { _ in print(x) }
 _ = JSObjectRef.global.document.object!.addEventListener!("visibilitychange", closure)
}
observe(x: 1)
observe(x: 2)

In your example, closure will be deinitialized when returning from observer function because call of addEventListener won't increment Swift-side reference count. However, closure can be called after deinit from JavaScript side and that call can't succeed because it's already released.

So users should call release after un-registering event listener manually.

BTW, it may be helpful if JavaScriptKit warns when JSClosure is deinited before release called

@kateinoigakukun

Copy link

Member

kateinoigakukun commented Aug 9, 2020

As @j-f1 said, after FinalizationRegistry will be available on all platforms, these kinds of manual memory management won't be necessary.

@MaxDesiatov

Copy link

Member Author

MaxDesiatov commented Aug 9, 2020 •

edited

Loading

In your example, closure will be deinitialized when returning from observer function because call of addEventListener won't increment Swift-side reference count. However, closure can be called after deinit from JavaScript side and that call can't succeed because it's already released.

That's exactly my point here. With this PR the call can't succeed and a user gets an error message about that, so the incorrect behavior becomes known. Without this PR the closure is deallocated anyway, the user doesn't get any error message, but the behavior is incorrect due to a single ObjectIdentifier value being reused for different closures. This is hard to diagnose and leads to many more subtle bugs in libraries that depend on JavaScriptKit. Without any error messages and the requirement for manual closure reference management, it's hard to do a full audit of the source code and eradicate these bugs. With the error message, even though it requires testing to trigger it at runtime, that at least becomes realistic.

FinalizationRegistry will be available on all platforms

I understand that, and it would be fantastic when that's the case. Unfortunately, we don't have it in all browsers, and even after (and if) it becomes available, we'll have to wait for a few years before new versions of Safari supporting it become widely adopted for this to work. The problem I described above is reproducible now and I hope we can at least let our users know about it. And it would also help us do the closure reference management audit in our code that depends on JavaScriptKit.

@MaxDesiatov MaxDesiatov requested a review from kateinoigakukun

August 9, 2020 14:20

@MaxDesiatov

Copy link

Member Author

MaxDesiatov commented Aug 9, 2020

BTW, it may be helpful if JavaScriptKit warns when JSClosure is deinited before release called

What do you think about having fatalError in deinit that's triggered if release wasn't called manually? I think it would be equivalent to the PR in its current form in the behavior (manifested in a crash), but I personally find managing it with references and relying on deinit doing the work easier than manual release calls.

@kateinoigakukun

Copy link

Member

kateinoigakukun commented Aug 9, 2020 •

edited

Loading

@MaxDesiatov

At first, the lifecycle of closure is tied to the GC of JavaScript also, so I think closures should not be released on the Swift lifecycle. We should provide a way to manage to sync thier lifecycle system. That's release method.

I think it would be equivalent to the PR in its current form in the behavior (manifested in a crash)

I think they are not equivalent behavior. With your proposal, the users will get error message when calling the closure from JavaScript. But with my proposal, the users will get error message when deallocating closure. The latter approach can tell error message to the user earlier.

@MaxDesiatov

Copy link

Member Author

MaxDesiatov commented Aug 9, 2020

But with my proposal, the users will get error message when deallocating closure. The latter approach can tell error message to the user earlier.

Oh, that makes sense, sorry for the confusion. Would fatalError in deinit, as I suggested in the previous comment, work well in your opinion?

@kateinoigakukun

Copy link

Member

kateinoigakukun commented Aug 9, 2020

Would fatalError in deinit, as I suggested in the previous comment, work well in your opinion?

Yes, that's exactly what I was thinking 😄

@MaxDesiatov


 Add isRelease property to JSClosure

@MaxDesiatov

Copy link

Member Author

MaxDesiatov commented Aug 10, 2020

@kateinoigakukun that's done now. Uncovered an issue in the existing ObjectRef_Lifetime test by the way 🙂

kateinoigakukun

kateinoigakukun approved these changes

Aug 11, 2020

View reviewed changes

Copy link

Member

@kateinoigakukun kateinoigakukun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kateinoigakukun kateinoigakukun merged commit 5e79c1f into master

Aug 11, 2020

@kateinoigakukun kateinoigakukun deleted the jsclosure-deinit branch

August 11, 2020 04:27

Labels

None yet

4 participants

@MaxDesiatov @j-f1 @kateinoigakukun

Uh oh!

Change JSClosure.release to deinit #33

Change JSClosure.release to deinit #33

Uh oh!

Conversation

@MaxDesiatov MaxDesiatov commented Aug 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MaxDesiatov commented Aug 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

j-f1 commented Aug 8, 2020

Uh oh!

@kateinoigakukun kateinoigakukun left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kateinoigakukun commented Aug 9, 2020

Uh oh!

MaxDesiatov commented Aug 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MaxDesiatov commented Aug 9, 2020

Uh oh!

kateinoigakukun commented Aug 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MaxDesiatov commented Aug 9, 2020

Uh oh!

kateinoigakukun commented Aug 9, 2020

Uh oh!

MaxDesiatov commented Aug 10, 2020

Uh oh!

@kateinoigakukun kateinoigakukun left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Change `JSClosure.release` to `deinit` #33

Change `JSClosure.release` to `deinit` #33

@MaxDesiatov MaxDesiatov commented Aug 4, 2020 •

edited

Loading

MaxDesiatov commented Aug 4, 2020 •

edited

Loading

@kateinoigakukun kateinoigakukun left a comment •

edited

Loading

MaxDesiatov commented Aug 9, 2020 •

edited

Loading

kateinoigakukun commented Aug 9, 2020 •

edited

Loading